-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KV and OS Compression and Metadata #1034
Conversation
scottf
commented
Oct 30, 2023
•
edited
Loading
edited
- Add compression and metadata support to KV and OS
- Some test efficiency improvements.
- JsonValue utility improvements to support toString for KV and OS objects
- Removal of EXPERIMENTAL label on Object Store
Also less generic test names for efficiency
*/ | ||
public long getMaxBucketSize() { | ||
return sc.getMaxBytes(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here because it's common to KV and OS
return sc.getCompressionOption() == JS_COMPRESSION_YES; | ||
} | ||
|
||
protected static abstract class Builder<B, FC> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here b/c common to KV and OS. Should have been this way originally.
*/ | ||
public long getMaxBucketSize() { | ||
return sc.getMaxBytes(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common to KV and OS
*/ | ||
public Republish getRepublish() { | ||
return sc.getRepublish(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common to KV and OS
@@ -346,7 +346,7 @@ public KeyValueConfiguration build() { | |||
.build()); | |||
} | |||
} | |||
else if (sources.size() > 0) { | |||
else if (!sources.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intellij hounds me about these. isEmpty checks the internal size variable. not sure why it always wants this, but it's the same amount of function calls
public long getMaxBucketSize() { | ||
return sc.getMaxBytes(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common to KV and OS
@@ -668,7 +668,6 @@ enum Status { | |||
|
|||
/** | |||
* Gets a context for working with an Object Store. | |||
* OBJECT STORE IMPLEMENTATION IS EXPERIMENTAL AND SUBJECT TO CHANGE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing compatibility tests, can remove the EXPERIMENTAL label.
mb.put("metaData", getMetadata()); | ||
return mb.toJsonValue(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this just to have easy toString()
*/ | ||
public Mirror getMirror() { | ||
return sc.getMirror(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added missing getter
*/ | ||
public List<Source> getSources() { | ||
return sc.getSources(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added missing getter
@@ -157,8 +170,7 @@ public Builder(KeyValueConfiguration kvc) { | |||
* @return the builder | |||
*/ | |||
public Builder name(String name) { | |||
this.name = validateBucketName(name, true); | |||
return this; | |||
return super.name(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not have to override these, but it allows me to have more specific api comments.
@@ -442,7 +442,7 @@ public boolean isDiscardNewPerSubject() { | |||
} | |||
|
|||
/** | |||
* Metadata for the consumer | |||
* Metadata for the stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad old copy paste fixed.
@@ -49,6 +49,8 @@ public enum Type { | |||
public final Object object; | |||
public final Number number; | |||
|
|||
public final List<String> mapOrder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helps toString to run through the map keys in a specific order, instead of whatever the keySet iterator would give.
@@ -347,6 +347,10 @@ public static String variant(Object variant) { | |||
return variant == null ? NUID.nextGlobalSequence() : "" + variant; | |||
} | |||
|
|||
public static String variant() { | |||
return NUID.nextGlobalSequence(); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes in this class are part of making the tests more efficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM